Skip to content

[4.1] Disable auth binding code introduced with 4.1.1#37415

Merged
bembelimen merged 15 commits intojoomla:4.1-devfrom
SniperSister:4x-authproviderbind
Mar 30, 2022
Merged

[4.1] Disable auth binding code introduced with 4.1.1#37415
bembelimen merged 15 commits intojoomla:4.1-devfrom
SniperSister:4x-authproviderbind

Conversation

@SniperSister
Copy link
Contributor

@SniperSister SniperSister commented Mar 29, 2022

Pull Request for Issue #37413

Summary of Changes

Remove the auth binding field and checking code to disable the feature until a proper solution has been implemented see #37405.

Testing Instructions

See #34713

Actual result BEFORE applying this Pull Request

You are NOT logged into the site

Expected result AFTER applying this Pull Request

You are logged into the site

@brianteeman
Copy link
Contributor

Tested the plugin changes and they at least let me log back in again. Have not tested the script.php changes which I assume is to fix upgrades. Still doesn't address the myriad of other issues with this disaster of a code change

@zero-24
Copy link
Contributor

zero-24 commented Mar 29, 2022

I have tested this item ✅ successfully on deb4d5f

Confirmed the issue, tested the patch. Confirmed that the script.php checks the values and resets them back to empty when they are invalid


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37415.

@bayareajenn
Copy link

I have tested this item ✅ successfully on deb4d5f

I don't have a CA SSL cert so could not test Webauthn. I tested it the same way as 37416 and was able to duplicate the problem and then load the patch and login again.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37415.

@SniperSister SniperSister changed the title [4.1] Only bind users to primary auth providers [4.1] Disable auth binding code introduced with 4.1.1 Mar 30, 2022
@brianteeman
Copy link
Contributor

Canyou remove the new language strings as well please
COM_USERS_USER_FIELD_AUTHPROVIDER_DESC
COM_USERS_USER_FIELD_AUTHPROVIDER_LABEL

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Mar 30, 2022
@SniperSister
Copy link
Contributor Author

Canyou remove the new language strings as well please

Done! Also checked in with @tecpromotion as tt-coordinator.

@brianteeman
Copy link
Contributor

and remove
administrator/components/com_users/src/Field/PrimaryauthprovidersField.php

@brianteeman
Copy link
Contributor

components/com_users/tmpl/profile/edit.php
<?php if (count($this->twofactormethods) > 1 && !empty($this->twofactorform)) : ?>

not sure if that change is still needed

@brianteeman
Copy link
Contributor

libraries/src/Authentication/ProviderAwareAuthenticationPluginInterface.php

@brianteeman
Copy link
Contributor

libraries/src/Table/User.php

 * Updates auth provider of a user

@SniperSister
Copy link
Contributor Author

administrator/components/com_users/src/Field/PrimaryauthprovidersField.php
libraries/src/Authentication/ProviderAwareAuthenticationPluginInterface.php

Would leave those two files for now as removing them (and therefore adding them to the deletion list in script.php) would prevent us from re-using the same file names for the future patch.

@brianteeman
Copy link
Contributor

No you have to remove them (and come up with new names in future) otherwise you will be breaking semver when you change the contents of the library files.

@brianteeman
Copy link
Contributor

libraries/src/User/User.php
public function setAuthProvider($authProvider)

@brianteeman
Copy link
Contributor

brianteeman commented Mar 30, 2022

plugins/authentication/cookie/cookie.php
use Joomla\CMS\Authentication\ProviderAwareAuthenticationPluginInterface;
public static function isPrimaryProvider()

and the same in other plugins

@SniperSister
Copy link
Contributor Author

No you have to remove them (and come up with new names in future) otherwise you will be breaking semver when you change the contents of the library files.

Ok, fair comment, done!

@brianteeman
Copy link
Contributor

Thanks @SniperSister

@HLeithner
Copy link
Member

semver!!

where do you see a semver violation?

@brianteeman
Copy link
Contributor

semver!!

where do you see a semver violation?

it should be properly marked as deprecated so that an ide etc will spot it.

@HLeithner
Copy link
Member

hat an ide etc will spot it.

The interface is deprecated all other parts get removed because not covered by our semver promise. Also semver doesn't allow us to add deprecations in bugfix releases. that's the reason David added the comment "Please note: might be deprecated with Joomla 4.2"

or do you mean something different?

@bembelimen
Copy link
Contributor

Thanks everyone for this good collaboration.
@nikosdion and @brianteeman are you both ok with the solution?

@nikosdion
Copy link
Contributor

@bembelimen I am OK with this solution.

@brianteeman
Copy link
Contributor

@bembelimen seems ok.

@bembelimen bembelimen merged commit 2377ea6 into joomla:4.1-dev Mar 30, 2022
@bembelimen
Copy link
Contributor

Thank you all!

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Mar 30, 2022

@SniperSister @bembelimen the binding issue has extreme similarities to the Media Manager Adapters, maybe you should take a look how @joomdonation @Fedik @laoneo and me tackled that binding and get some inspiration...

Basically #34069 and #33724 (maybe a couple more)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.